Allow specifiying data areas 1-4 when generating telemetry logs#2755
Allow specifiying data areas 1-4 when generating telemetry logs#2755dsfw-opensource wants to merge 1 commit intolinux-nvme:masterfrom
Conversation
Allow users to specify data areas 1-4 when generating telemetry logs. Previously, the command is rejected if it is not 1 or 2. When users specify data area N, data areas <= N now get parsed into the output. Closes: linux-nvme#2754 Signed-off-by: dsfw-opensource <[email protected]>
|
|
||
| if (pevent_fifos_object != NULL && root != NULL) { | ||
| const char *data_area = (poffsets->data_area == 1 ? STR_DA_1_EVENT_FIFO_INFO : | ||
| const char *data_area = (poffsets->data_area == DATA_AREA_1 ? STR_DA_1_EVENT_FIFO_INFO : |
There was a problem hiding this comment.
If the data area is 3 or 4 then the data area 2 event fifo information used but is it expected as correct?
There was a problem hiding this comment.
Also there are checkpatch.pl 2 warnings as below.
tokunori@tokunori-desktop:~/nvme-cli-2$ ~/linux/scripts/checkpatch.pl -g HEAD
WARNING: 'specifiying' may be misspelled - perhaps 'specifying'?
#4:
Subject: [PATCH] Allow specifiying data areas 1-4 when generating telemetry
^^^^^^^^^^^
WARNING: line length of 104 exceeds 100 columns
#68: FILE: plugins/ocp/ocp-telemetry-decode.c:1226:
+ const char *data_area = (poffsets->data_area == DATA_AREA_1 ? STR_DA_1_EVENT_FIFO_INFO :
total: 0 errors, 2 warnings, 120 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
Commit ffa58cc6d956 ("Allow specifiying data areas 1-4 when generating telemetry logs") has style problems, please review.
NOTE: Ignored message types: EMAIL_SUBJECT FILE_PATH_CHANGES GERRIT_CHANGE_ID GIT_COMMIT_ID NOT_UNIFIED_DIFF
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.There was a problem hiding this comment.
I believe the behavior makes sense. Is there a maintainer I can contact to confirm if this is the intended behavior? The specification is not very clear, but I based it on the original behavior where, if a user requested data area 2, they also received data areas 1 and 2 parsed into a human-readable format. Following that logic, I would expect data area 3 to include data areas 1 and 2 parsed into a human-readable format, with data areas >= 3 remaining in the raw binary log since it is vendor-specific.
There was a problem hiding this comment.
The base spec has the following on telemetry log:
I assume the ocp spec will follow the base spec, so no need to include the areas 1,2 if 3 is selected.
To clarify this you could contact the authors of the ocp spec though.
There was a problem hiding this comment.
Ok, I will try contacting the authors.
| if (root != NULL && pstats_array != NULL) { | ||
| const char *pdata_area = | ||
| (poffsets->data_area == 1 ? STR_DA_1_STATS : STR_DA_2_STATS); | ||
| (poffsets->data_area == DATA_AREA_1 ? STR_DA_1_STATS : STR_DA_2_STATS); |
There was a problem hiding this comment.
Is this also correct to use the data area 2 statistics for the data area 3 or 4?
There was a problem hiding this comment.
Data area 3 and 4 are vendor-specific. This is just to ensure data area 2 also gets parsed.
| if (options->data_area >= DATA_AREA_2) { | ||
| //Set the DA to 2 | ||
| offsets.data_area = 2; | ||
| offsets.data_area = DATA_AREA_2; |
There was a problem hiding this comment.
Is this also really expected for the data area 3 and 4?
There was a problem hiding this comment.
Anyway if for the option data area 3 or 4 specified it is expected to use the data area 2 internally then the description comment needed I think.
There was a problem hiding this comment.
Yes, I will add a comment.
| if (options->data_area == 2) { | ||
| offsets.data_area = 2; | ||
| if (options->data_area >= DATA_AREA_2) { | ||
| offsets.data_area = DATA_AREA_2; |
There was a problem hiding this comment.
Not necessary to set DATA_AREA_3 or DATA_AREA_4 for the data area 3 and 4?
There was a problem hiding this comment.
Data areas 3 and 4 are vendor specific.
| struct nvme_ocp_telemetry_offsets offsets = { 0 }; | ||
|
|
||
| offsets.data_area = 1;// Default DA - DA1 | ||
| offsets.data_area = DATA_AREA_1;// Default DA - DA1 |
There was a problem hiding this comment.
Below seems better for the comment.
- offsets.data_area = DATA_AREA_1;// Default DA - DA1
+ offsets.data_area = DATA_AREA_1; /* Default DA - DA1 */| @@ -1509,7 +1510,7 @@ int print_ocp_telemetry_normal(struct ocp_telemetry_parse_options *options) | |||
| //Set DA to 1 and get offsets | |||
There was a problem hiding this comment.
Below seems better for the comment. Anyway a space needed after // I think.
- //Set DA to 1 and get offsets
+ /* Set DA to 1 and get offsets */There was a problem hiding this comment.
Sure, I will change it.
|
Ok, I might of had a misunderstanding of the command. After thinking about it some more, it makes sense that |
3 participants
Allow users to specify data areas 1-4 when generating telemetry logs. Previously, the command is rejected if it is not 1 or 2. When users specify data area N, data areas <= N now get parsed into the output.
Closes: #2754